Fixed the yes/no prompt in verdi computer delete by Krishnabhujade · Pull Request #6624 · aiidateam/aiida-core

Krishnabhujade · 2024-11-20T14:19:26Z

This PR fixes the "yes/no" prompt in verdi computer delete

agoscinski · 2024-11-25T12:35:43Z

I am sorry for the back and forth but reading now the conversation in issue #6422 more carefully and looking at the deletion in other parts of the code. We can change it to [y/N], this gives us consistency over the CLI which I think is nice. We use click.confirm in other parts of the code for this.

Krishnabhujade · 2024-11-25T16:18:31Z

I am sorry for the back and forth but reading now the conversation in issue #6422 more carefully and looking at the deletion in other parts of the code. We can change it to [y/N], this gives us consistency over the CLI which I think is nice. We use click.confirm in other parts of the code for this.

Ohk @agoscinski I will do '[y/N]'

Krishnabhujade · 2024-11-25T16:28:16Z

@agoscinski
I am facing some problem in setting up the codebase locally

agoscinski · 2024-11-25T16:49:18Z

No worries I take over. It should now work.

Krishnabhujade · 2024-11-25T17:00:04Z

I want to contribute more to organisation so I want to install the codebase locally

agoscinski · 2024-11-26T09:34:17Z

Sure can you contact me on https://aiida.discourse.group ?
There is guide to install https://aiida.readthedocs.io/projects/aiida-core/en/latest/installation/guide_quick.html
There is also a development guide but it is a bit outdated https://github.com/aiidateam/aiida-core/wiki/Development-environment#using-python-virtual-environment

codecov · 2024-11-26T10:10:31Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.59%. Comparing base (474e0fa) to head (183465f).
⚠️ Report is 53 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6624      +/-   ##
==========================================
+ Coverage   78.58%   78.59%   +0.01%     
==========================================
  Files         567      567              
  Lines       43086    43087       +1     
==========================================
+ Hits        33856    33861       +5     
+ Misses       9230     9226       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Krishnabhujade · 2024-11-26T14:57:53Z

Thank you sir

khsrali

Thanks a lot @Krishnabhujade and @agoscinski ,
Some minor comments

khsrali · 2024-11-28T09:22:10Z

+    def test_computer_delete_nonexisting_computer(self):
+        """Test if `verdi computer delete` command works correctly for a nonexisting computer"""
+        # See if the command complains about not getting an nonexisting computer
+        self.cli_runner(computer_delete, ['computer_that_does_not_exist'], raises=True)


Please check explicitly if the raise is due to NotExistent.
Also please pass user_input=y I know here it might not matter because the query comes before click.confirm,
however, later if someone changes the order of the logic in the code then test might silently pass..
Better to be precise.
Thanks

We cannot check the actual exception at the CLI level, we only get a system exit. I added assert for the error message that relate to that exception.

khsrali · 2024-11-28T09:39:55Z

-            orm.Computer.collection.get(label=label)
+        # Safety check in case of --dry-run
+        options = [label, '--dry-run']
+        self.cli_runner(computer_delete, options)


Suggested change

self.cli_runner(computer_delete, options)

self.cli_runner(computer_delete, options, user_input="y")

khsrali · 2024-11-28T09:43:25Z

-        # Setup a computer to delete during the test
-        label = 'computer_for_test_label'
-        orm.Computer(
+    def test_computer_delete_existing_computer(self):


I found this test name kinda misleading for it's purpose

Suggested change

def test_computer_delete_existing_computer(self):

def test_computer_do_not_delete(self):

named it test_computer_delete_existing_computer_not_deleted do be consistent with the test name of the other one

khsrali

Thanks @agoscinski, all good!

khsrali · 2025-04-23T16:53:48Z

-            orm.load_code(c_label)
-
-    def test_computer_delete(self):
-        """Test if 'verdi computer delete' command works"""


I don't understand why this test is deleted.
Where do we actually check if the the command would function, when deleting a computer that has no associated nodes?

true the diff looks weird now. I just redo this PR when I have time

yes this test exists I restructured the tests a bit, made them more granular. I restructured the tests test_computer_delete and test_computer_delete_with_nodes to more granular test cases test_computer_delete_existing_computer_successful, test_computer_delete_existing_computer_not_deleted and test_computer_delete_nonexisting_computer

Ok still I don't see a test that would delete a computer without associated nodes.

Like this:

def test_computer_delete_no_associated nodes(self): orm.Computer(..).store().configure() self.cli_runner(computer_delete, [label]) with pytest.raises(NotExistent): orm.Computer.collection.get(label=label)

khsrali

Alex we need a test like this to make sure not having associated nodes will not prompt for an input and can function as before:

  def test_computer_delete_no_associated nodes(self):
    orm.Computer(..).store().configure()
    self.cli_runner(computer_delete, [label])
    with pytest.raises(NotExistent):
      orm.Computer.collection.get(label=label)

khsrali · 2025-05-06T08:43:26Z

+    # Ensures that 'yes' works for backwards compatibility, see issue #6422
+    @pytest.mark.parametrize('user_input', ['y', 'yes'])
+    def test_computer_delete_existing_computer_successful(self, user_input):
+        """Test if `verdi computer delete` successfully deletes when applied on an existing computer."""


Suggested change

"""Test if `verdi computer delete` successfully deletes when applied on an existing computer."""

"""Test if `verdi computer delete` successfully deletes when applied on an existing computer, which has associated nodes."""

There is no difference if you delete a node that is associated to something

$ node delete 8 Report: 1 Node(s) marked for deletion Warning: YOU ARE ABOUT TO DELETE 1 NODES! THIS CANNOT BE UNDONE!

khsrali · 2025-05-06T08:58:16Z

-            orm.load_code(c_label)
-
-    def test_computer_delete(self):
-        """Test if 'verdi computer delete' command works"""


Ok still I don't see a test that would delete a computer without associated nodes.

Like this:

def test_computer_delete_no_associated nodes(self): orm.Computer(..).store().configure() self.cli_runner(computer_delete, [label]) with pytest.raises(NotExistent): orm.Computer.collection.get(label=label)

khsrali

LGTM!

The yes/no prompt of `verdi computer delete` is the only prompt in the CLI that does not accept 'y' as valid answer but only 'yes'. This has been aligned with the other prompts to also support 'y'. Restructure the tests `test_computer_delete` and `test_computer_delete_with_nodes` to more granular test cases `test_computer_delete_existing_computer_successful`, `test_computer_delete_existing_computer_not_deleted` and `test_computer_delete_nonexisting_computer` Cleanup names in tests.

…e associated to the computer

The mock timeout is reached in CI so we increase it.

khsrali · 2025-05-06T15:20:54Z

What's happening here after my approval 👀

agoscinski · 2025-05-06T17:38:51Z

Everything is fine here, please Sir review my next PR.

agoscinski reviewed Nov 25, 2024

View reviewed changes

Comment thread src/aiida/cmdline/commands/cmd_computer.py Outdated

agoscinski force-pushed the fix/yes/no-prompt-verdi-computer-delete branch from ce1ff59 to ccc2832 Compare November 25, 2024 12:37

agoscinski reviewed Nov 25, 2024

View reviewed changes

Comment thread src/aiida/cmdline/commands/cmd_computer.py Outdated

agoscinski force-pushed the fix/yes/no-prompt-verdi-computer-delete branch from 1225c23 to 1f5aea4 Compare November 25, 2024 15:42

agoscinski force-pushed the fix/yes/no-prompt-verdi-computer-delete branch 2 times, most recently from e3f8bef to a33cfb2 Compare November 26, 2024 09:33

agoscinski requested a review from khsrali November 26, 2024 14:40

khsrali requested changes Nov 28, 2024

View reviewed changes

agoscinski self-assigned this Feb 19, 2025

agoscinski force-pushed the fix/yes/no-prompt-verdi-computer-delete branch 2 times, most recently from 85d7c21 to 6c69993 Compare March 6, 2025 07:30

agoscinski requested a review from khsrali March 6, 2025 08:51

khsrali approved these changes Mar 7, 2025

View reviewed changes

agoscinski force-pushed the fix/yes/no-prompt-verdi-computer-delete branch from 6c69993 to e14a2bc Compare March 25, 2025 09:12

agoscinski moved this to Done in aiida-core v2.7.0 Mar 25, 2025

khsrali requested changes Apr 23, 2025

View reviewed changes

agoscinski moved this from Done to In progress in aiida-core v2.7.0 May 5, 2025

agoscinski removed this from aiida-core v2.7.0 May 5, 2025

agoscinski added this to aiida-core v2.7.1 May 5, 2025

agoscinski moved this to In review in aiida-core v2.7.1 May 5, 2025

agoscinski force-pushed the fix/yes/no-prompt-verdi-computer-delete branch from a674a64 to 35f14e3 Compare May 6, 2025 05:51

agoscinski requested a review from khsrali May 6, 2025 05:52

agoscinski removed this from aiida-core v2.7.1 May 6, 2025

agoscinski added this to aiida-core v2.7.0 May 6, 2025

agoscinski moved this to In review in aiida-core v2.7.0 May 6, 2025

agoscinski force-pushed the fix/yes/no-prompt-verdi-computer-delete branch 2 times, most recently from a48331c to 030627a Compare May 6, 2025 07:03

khsrali requested changes May 6, 2025

View reviewed changes

khsrali approved these changes May 6, 2025

View reviewed changes

agoscinski force-pushed the fix/yes/no-prompt-verdi-computer-delete branch 2 times, most recently from 048a9f0 to 699f27f Compare May 6, 2025 12:23

Krishnabhujade and others added 3 commits May 6, 2025 16:44

Remove prompt verdi delete computer without prompt when no nodes ar…

841172e

…e associated to the computer

Increase check count in MockProcess

183465f

The mock timeout is reached in CI so we increase it.

agoscinski force-pushed the fix/yes/no-prompt-verdi-computer-delete branch from 699f27f to 183465f Compare May 6, 2025 14:44

agoscinski merged commit 71fc14f into aiidateam:main May 6, 2025
11 checks passed

github-project-automation Bot moved this from In review to Done in aiida-core v2.7.0 May 6, 2025

	self.cli_runner(computer_delete, options)
	self.cli_runner(computer_delete, options, user_input="y")

	def test_computer_delete_existing_computer(self):
	def test_computer_do_not_delete(self):

	"""Test if `verdi computer delete` successfully deletes when applied on an existing computer."""
	"""Test if `verdi computer delete` successfully deletes when applied on an existing computer, which has associated nodes."""

Conversation

Krishnabhujade commented Nov 20, 2024

Uh oh!

agoscinski commented Nov 25, 2024

Uh oh!

Uh oh!

Uh oh!

Krishnabhujade commented Nov 25, 2024

Uh oh!

Krishnabhujade commented Nov 25, 2024

Uh oh!

agoscinski commented Nov 25, 2024

Uh oh!

Krishnabhujade commented Nov 25, 2024

Uh oh!

agoscinski commented Nov 26, 2024

Uh oh!

codecov Bot commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Krishnabhujade commented Nov 26, 2024

Uh oh!

khsrali left a comment

Choose a reason for hiding this comment

Uh oh!

khsrali Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khsrali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khsrali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khsrali left a comment

Choose a reason for hiding this comment

Uh oh!

khsrali commented May 6, 2025

Uh oh!

Uh oh!

agoscinski commented May 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Nov 26, 2024 •

edited

Loading

khsrali Nov 28, 2024 •

edited

Loading